Dx 2688 Conferences Integration Tests by brianluisgomez · Pull Request #98 · Bandwidth/python-sdk

brianluisgomez · 2022-08-02T04:41:16Z

No description provided.

test/integration/test_conferences.py

ajrice6713 · 2022-08-03T20:04:55Z

test/integration/test_conferences.py

+        time.sleep(2)
+        list_conferences_response = self.conference_api_instance.list_conferences(BW_ACCOUNT_ID, _return_http_data_only=False)
+
+        time.sleep(2)


Why the duplicate sleep?

test/integration/test_conferences.py

ajrice6713 · 2022-08-03T20:17:14Z

test/integration/test_conferences.py

+        # Get Conference Information
+        """Validate a Get Conference Information Request
+        """


Are these random docstrings lefotver from copy/pasting functions?

test/integration/test_conferences.py

bpateldx · 2022-08-04T14:13:56Z

test/integration/test_conferences.py

+testRecordId = "Recording-Id"
+
+
+class ConferencesIntegration(unittest.TestCase):


Can some of the refactoring done for Calls API Test be applied here as well?

bpateldx · 2022-08-04T14:21:54Z

test/integration/test_conferences.py

+                elif get_call_response[0].state == 'complete':
+                    callIdArray.remove(callId)
+            callIdArray.clear()
+        pass         


Refactoring This function is used in multiple Tests, opportunity to remove duplication. We can create a base Test Class with common functionality including this function and use where applicable for Calls API usage.

bpateldx

Nice work - removing ~100 lines of code from Test class - inheritance is worth trying if there is more commonality.

ajrice6713 · 2022-08-08T15:00:04Z

test/integration/test_conferences.py

+from test.utils.env_variables import *
+from test.utils.call_cleanup import callCleanup
+import json
+import time
+from typing import List, Tuple
+import unittest
+from hamcrest import assert_that, has_properties, not_none, instance_of, greater_than
+
+
+import bandwidth
+from bandwidth.api import calls_api
+from bandwidth.model.create_call import CreateCall
+from bandwidth.model.create_call_response import CreateCallResponse
+from bandwidth.model.call_state import CallState
+from bandwidth.model.call_state_enum import CallStateEnum
+from bandwidth.model.update_call import UpdateCall
+from bandwidth.model.redirect_method_enum import RedirectMethodEnum
+from bandwidth.api import conferences_api
+from bandwidth.model.conference_state_enum import ConferenceStateEnum
+from bandwidth.model.conference_recording_metadata import ConferenceRecordingMetadata
+from bandwidth.model.update_conference import UpdateConference
+from bandwidth.model.update_conference_member import UpdateConferenceMember
+from bandwidth.model.file_format_enum import FileFormatEnum
+from bandwidth.rest import RESTClientObject, RESTResponse
+from bandwidth.exceptions import ApiException, UnauthorizedException, ForbiddenException, NotFoundException


Order of imports in python according to Pep8 is

built in libraries

3rd party installed libraries

local modules

import time import hamcrest import bandwidth

IMO we should implement autopep8 or black to run on all PRs and autoformat code

OK, I changed the order of imports but I think implementing something. like autopep8 should be moved to a new card

ajrice6713 · 2022-08-08T15:03:12Z

test/integration/test_conferences.py

+        # Two Valid API Clients
+        self.calls_api_instance = calls_api.CallsApi(api_client)
+        self.conference_api_instance = conferences_api.ConferencesApi(api_client)
+
+        # Unauthorized API Client
+
+        unauthorizedConfiguration = bandwidth.Configuration(


Inconsistent # comment spacing - IMO your variable names are specific enough that we dont need these comments

Good catch, I removed most of those comments.

test/integration/test_conferences.py

ajrice6713 · 2022-08-08T15:04:33Z

test/integration/test_conferences.py

+    def tearDown(self):
+        callCleanup(self)
+
+    def assertApiException(self, context: ApiException, expectedException: ApiException, expected_status_code: int):


We should probably move this to a test utils module or something since its reused in multiple tests

Doesnt need to get done here - just thinking out loud

I like this idea too, but I think it needs to be on a separate card since we'll also have to modify all the other integration tests that use this function so it's a bit out of the scope of the conferences card

ajrice6713 · 2022-08-08T15:07:45Z

test/integration/test_conferences.py

+        # List Conferences
+        list_conferences_response = self.conference_api_instance.list_conferences(BW_ACCOUNT_ID, _return_http_data_only=False)
+
+        assert_that(list_conferences_response[1], 200)


More thinking out loud - I desipise the fact that _return_http_data_only returns a tuple and not a named tuple - if we can implement this without breaking anything (which I think we can since named tuples allow you to reference the key) - it might be a nice addition. Calling the index feels very cheesy to me

I like this too, but same as above I think this with the other improvements can be added to another card that we can do next sprint or at some point

ajrice6713 · 2022-08-08T15:09:30Z

test/integration/test_conferences.py

+        # Get Conference Member
+        """Validate a GET Conference Member
+        """
+        get_conference_member_response = self.conference_api_instance.get_conference_member(BW_ACCOUNT_ID, conferenceId, callId,  _return_http_data_only=False)
+        assert_that(get_conference_member_response[1], 200)
+        assert_that(get_conference_member_response[0].conference_id, conferenceId)
+        assert_that(get_conference_member_response[0].call_id, callId)
+
+        # Update Conference Member
+        """Validate an Update Conference Member Request
+        """
+
+        time.sleep(self.TEST_SLEEP)


More inconsistent comment spacing

Should be good now

ajrice6713

Looks good other than the imports spacing

test/integration/test_conferences.py

Co-authored-by: AJ Rice <53190766+ajrice6713@users.noreply.github.com>

DX-2688

7e1c593

brianluisgomez requested a review from a team August 2, 2022 04:41

brianluisgomez changed the base branch from main to feature/openapi-generator-sdk August 2, 2022 13:25

Merge branch 'feature/openapi-generator-sdk' into DX-2688

c4b7743

ajrice6713 requested changes Aug 3, 2022

View reviewed changes

bpateldx requested changes Aug 4, 2022

View reviewed changes

DX-2688 Added utils and other cleanup

1a4f530

brianluisgomez requested review from ajrice6713 and bpateldx August 4, 2022 18:17

bpateldx approved these changes Aug 4, 2022

View reviewed changes

brianluisgomez added 5 commits August 5, 2022 09:29

Merge branch 'feature/openapi-generator-sdk' into DX-2688

937b0f6

Update test.yaml

d78f144

Update test_recordings.py

d45dfc5

Merge branch 'feature/openapi-generator-sdk' into DX-2688

2f5ffe4

Merge branch 'feature/openapi-generator-sdk' into DX-2688

8914950

ajrice6713 requested changes Aug 8, 2022

View reviewed changes

Update test_conferences.py

e9a1b5a

brianluisgomez requested a review from ajrice6713 August 8, 2022 16:50

ajrice6713 requested changes Aug 8, 2022

View reviewed changes

test/integration/test_conferences.py Show resolved Hide resolved

Update test/integration/test_conferences.py

462f6b9

Co-authored-by: AJ Rice <53190766+ajrice6713@users.noreply.github.com>

ajrice6713 approved these changes Aug 8, 2022

View reviewed changes

brianluisgomez merged commit d257d1a into feature/openapi-generator-sdk Aug 8, 2022

brianluisgomez deleted the DX-2688 branch August 8, 2022 17:30

		testRecordId = "Recording-Id"


		class ConferencesIntegration(unittest.TestCase):

Conversation

brianluisgomez commented Aug 2, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpateldx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajrice6713 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants